refactor: unified polymorphic domain + registry#1983
Conversation
Split `RegistryId` into a union of `ENSv1RegistryId`, `ENSv1VirtualRegistryId`,
and `ENSv2RegistryId`. Add corresponding `makeENSv1RegistryId`,
`makeENSv2RegistryId`, and `makeENSv1VirtualRegistryId` constructors, and keep
`makeRegistryId` as a union-returning helper for callsites that genuinely can't
narrow (e.g. client-side cache key reconstruction).
Reshape `ENSv1DomainId` from `Node` to `${ENSv1RegistryId}/${node}` so ENSv1
domains are addressable in the same namegraph model as ENSv1VirtualRegistry.
`makeENSv1DomainId` now takes `(AccountId, Node)` — breaking change for all
callers.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replace the split `v1_domains` + `v2_domains` tables with a single polymorphic `domains` table keyed by `DomainId` and discriminated by `domainType` enum (`"ENSv1Domain"` | `"ENSv2Domain"`). Drop `domain.parentId`; ENSv1 parent traversal now flows through `registryCanonicalDomain` uniformly with ENSv2. `tokenId` becomes nullable (non-null iff ENSv2). Make `registries` polymorphic: add `registryType` enum (`"ENSv1Registry"` | `"ENSv1VirtualRegistry"` | `"ENSv2Registry"`), add nullable `node` column (non-null iff virtual), replace the unique `(chainId, address)` constraint with a plain index so virtual Registries keyed by node can share (chainId, address) with their concrete parent. Widen `registryCanonicalDomain.domainId` from `ENSv2DomainId` to the unified `DomainId`. Add `getENSv1RootRegistryId` / `maybeGetENSv1RootRegistryId` / `maybeGetENSv1Registry` helpers mirroring the v2 equivalents; narrow v2 helpers to use `makeENSv2RegistryId`. Update the ensdb-sdk drizzle test to reference the unified `domain` export. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Extend `managed-names.ts`: `CONTRACTS_BY_MANAGED_NAME` now maps `Name` to
`{ registry, contracts }`, `getManagedName(contract)` returns
`{ name, node, registry }` so any Registrar / Controller / NameWrapper handler
can resolve the concrete ENSv1 Registry that governs its namegraph. Add the
ENS Root (`""`) Managed Name group covering the mainnet ENSv1Registry and
ENSv1RegistryOld; include each shadow Registry (Basenames, Lineanames) in its
respective Managed Name group. Groups for namespaces that don't ship a given
shadow Registry are omitted entirely.
ENSv1 `handleNewOwner`: upsert the concrete ENSv1 Registry row, pick
`parentRegistryId` as the concrete Registry when `parentNode` is the Managed
Name and as an `ENSv1VirtualRegistry` keyed by `parentNode` otherwise. When
the parent is virtual, also upsert the virtual Registry row and the
`registryCanonicalDomain` self-link so reverse traversal works uniformly with
ENSv2. Combine domain upsert with `rootRegistryOwner` update into one query
via `onConflictDoUpdate`. Canonicalize ENSv1Registry / ENSv1RegistryOld events
through `getManagedName(...).registry` — ENSRegistryWithFallback proxies
reads, so both contracts face the same logical namegraph and should write into
the same Registry ID.
All remaining v1 handlers (Transfer / NewTTL / NewResolver, BaseRegistrar,
NameWrapper, RegistrarController, protocol-acceleration ENSv1Registry /
ThreeDNSToken) update to the two-arg `makeENSv1DomainId(registry, node)`.
ENSv2 `handleRegistrationOrReservation`: switch `makeRegistryId` to
`makeENSv2RegistryId`, add `type: "ENSv2Registry"` to the Registry insert and
`type: "ENSv2Domain"` to the Domain insert.
Update `domain-db-helpers.materializeENSv1DomainEffectiveOwner` to write
through the unified `domain` table. Update the managed-names test to assert
the new `registry` return field.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Context & loaders
- Drop `v1CanonicalPath` + `v2CanonicalPath` loaders in favour of a single
`canonicalPath` loader backed by `getCanonicalPath(domainId)`.
Canonical path
- Replace `getV1CanonicalPath` + `getV2CanonicalPath` with a single recursive
CTE over `domain` + `registryCanonicalDomain`. Recursion terminates naturally:
roots have no `registryCanonicalDomain` entry, so the JOIN fails when we
reach one. Canonicality is decided by the final `tld.registry_id === root`
check. MAX_DEPTH guards against corrupted state.
Interpreted-name lookup (`get-domain-by-interpreted-name.ts`)
- Collapse the ENSv1 / ENSv2 branches into one `traverseFromRoot(root, name)`
helper. Both lineages hop via `domain.subregistryId` (ENSv1 Domains now set
this to their managed VirtualRegistry, symmetric with ENSv2 domains' declared
subregistries). The starting root picks v1 vs v2 lineage; v1 and v2 registry
IDs are disjoint, so no cross-contamination.
Find-domains layers
- `base-domain-set.ts`: single select over `domain`; `parentId` derived via
`registryCanonicalDomain` uniformly for v1 and v2.
- `filter-by-registry.ts`: simplify comment (no v1/v2 distinction).
- `filter-by-canonical.ts`: all domains have a `registryId` now; canonicality
reduces to `INNER JOIN` against the canonical-registries CTE.
- `filter-by-name.ts`: collapse `v1DomainsByLabelHashPath` +
`v2DomainsByLabelHashPath` into one CTE over `registryCanonicalDomain`.
- `canonical-registries-cte.ts`: union v1 + v2 roots as base cases; recursive
step uses `d.subregistry_id` uniformly.
Schemas
- `schema/domain.ts`: `DomainInterfaceRef` becomes a loadable interface with a
single `ensDb.query.domain.findMany` loader. `DomainInterface = Omit<Domain,
"tokenId" | "node" | "rootRegistryOwnerId">`. Variant types tightened via
`RequiredAndNotNull` / `RequiredAndNull` to encode invariants
(`ENSv1Domain.{node: Node, tokenId: null}`;
`ENSv2Domain.{tokenId: bigint, node: null, rootRegistryOwnerId: null}`).
`parent` moves onto the interface via `ctx.loaders.canonicalPath`; expose
`ENSv1Domain.node` as a first-class GraphQL field.
- `schema/registry.ts`: new `RegistryInterfaceRef` with `ENSv1Registry`,
`ENSv1VirtualRegistry`, `ENSv2Registry` implementations; shared fields
(`id`, `type`, `contract`, `parents`, `domains`, `permissions`). `parents`
uses `eq(domain.subregistryId, parent.id)` for virtual v1 and v2 (both set
`subregistryId`), and `eq(domain.registryId, parent.id)` for concrete v1.
`ENSv1VirtualRegistryRef` exposes `node: Node`.
- `schema/query.ts`: `registry(by: {contract})` does a DB lookup filtered by
`type IN (ENSv1Registry, ENSv2Registry)` — virtual Registries share
`(chainId, address)` with their concrete parent and aren't addressable via
contract alone. Dev-only `v1Domains` / `v2Domains` filter by `d.type`.
- Swap `RegistryRef` → `RegistryInterfaceRef` in `query.ts` and
`registry-permissions-user.ts`.
- `schema/registration.ts`: `WrappedBaseRegistrarRegistration.tokenId` loads
the domain via the `DomainInterfaceRef` dataloader and reads `domain.node`.
Supporting changes
- `ensdb-sdk` schema: add `domain.node: Hex` (non-null iff ENSv1Domain).
- `ensindexer` ENSv1 `handleNewOwner`: write `node` on domain upsert and set
parent domain's `subregistryId` to the VirtualRegistry when upserting it
(so forward traversal + canonical-registries CTE work uniformly with v2).
- `ensnode-sdk`: add `RequiredAndNull<T, K>` helper type (symmetric to
`RequiredAndNotNull`) for encoding "null in this variant" invariants.
Regenerate pothos generated files (`schema.graphql`, `introspection.ts`).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 9d59225 The changes in this PR will be included in the next version bump. This PR includes changesets to release 23 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 5 minutes and 18 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR unifies ENSv1 and ENSv2 domain and registry data models into single polymorphic tables with type discriminators, consolidates canonical-path and domain-finding logic to operate on unified schemas, updates the GraphQL interface layer to expose polymorphic Registry and Domain types with a unified parent field, and refactors all indexer handlers and ID constructors to support registry-aware domain identifiers. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR refactors the ENS data model and API surface to unify ENSv1 + ENSv2 domains into a single polymorphic domain table, introduces a polymorphic Registry model (including ENSv1 virtual registries), and updates the indexer + ENS API to traverse/query the unified namegraph—targeting correctness for multi-registry conflicts (#205) and improving find-domains architecture/perf (#1877) while advancing DomainId terminology (#1511).
Changes:
- Unifies DB schema from
v1Domain/v2Domainintodomain(typed by enum), and makesregistrypolymorphic (ENSv1 concrete / ENSv1 virtual / ENSv2). - Changes ENSv1 identifiers to be registry-qualified (
ENSv1DomainId = ${ENSv1RegistryId}/${node}) and adds new RegistryId constructors. - Updates indexer handlers + ENS API (GraphQL schema, loaders, find-domains layers, canonical path + interpreted-name traversal) to operate on the unified model and expose new GraphQL interfaces/fields (
Registryinterface,Domain.parent).
Reviewed changes
Copilot reviewed 34 out of 36 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/enssdk/src/omnigraph/generated/schema.graphql | Updates generated GraphQL schema: Domain.parent, ENSv1Domain.node, Registry becomes an interface with ENSv1/virtual/v2 implementations. |
| packages/enssdk/src/omnigraph/generated/introspection.ts | Updates generated introspection JSON to match new interfaces/types. |
| packages/enssdk/src/lib/types/ensv2.ts | Introduces branded ENSv1RegistryId/ENSv2RegistryId/ENSv1VirtualRegistryId and changes ENSv1 domain id shape. |
| packages/enssdk/src/lib/ids.ts | Adds makeENSv1RegistryId / makeENSv2RegistryId / makeENSv1VirtualRegistryId; changes makeENSv1DomainId signature to include registry. |
| packages/ensnode-sdk/src/shared/types.ts | Adds RequiredAndNull TS helper used for discriminated polymorphic models. |
| packages/ensnode-sdk/src/shared/root-registry.ts | Adds ENSv1 root registry id helpers and switches ENSv2 root helper to makeENSv2RegistryId. |
| packages/ensdb-sdk/src/lib/drizzle.test.ts | Updates schema cloning tests to reflect table rename (domain). |
| packages/ensdb-sdk/src/ensindexer-abstract/ensv2.schema.ts | Core schema refactor: new domain + polymorphic registry, new enums, updated relations/indexes, registryCanonicalDomain.domainId to unified DomainId. |
| apps/ensindexer/src/plugins/protocol-acceleration/handlers/ThreeDNSToken.ts | Updates ENSv1 domain id creation to include registry. |
| apps/ensindexer/src/plugins/protocol-acceleration/handlers/ENSv1Registry.ts | Canonicalizes ENSv1 registry source and updates domain id creation to include registry. |
| apps/ensindexer/src/plugins/ensv2/handlers/ensv2/ENSv2Registry.ts | Writes ENSv2 domains into unified domain table and inserts registries with polymorphic type. |
| apps/ensindexer/src/plugins/ensv2/handlers/ensv1/RegistrarController.ts | Updates ENSv1 domain id creation to include registry from managed-name group. |
| apps/ensindexer/src/plugins/ensv2/handlers/ensv1/NameWrapper.ts | Updates ENSv1 domain id creation to include registry for wrapper events. |
| apps/ensindexer/src/plugins/ensv2/handlers/ensv1/ENSv1Registry.ts | Implements ENSv1 virtual registries + unified domain writes; canonicalizes ENSv1RegistryOld vs new registry. |
| apps/ensindexer/src/plugins/ensv2/handlers/ensv1/BaseRegistrar.ts | Retargets reads/writes to unified domain table and updates ENSv1 id creation. |
| apps/ensindexer/src/lib/managed-names.ts | Expands managed-name mapping to include concrete registry per group; adds ENS root group and shadow registries. |
| apps/ensindexer/src/lib/managed-names.test.ts | Updates tests for new registry return from getManagedName. |
| apps/ensindexer/src/lib/ensv2/domain-db-helpers.ts | Updates effective-owner materialization to write unified domain table. |
| apps/ensapi/src/omnigraph-api/schema/registry.ts | Replaces Registry object with Registry interface + ENSv1/virtual/v2 object types; updates parent/domain connections. |
| apps/ensapi/src/omnigraph-api/schema/registry-permissions-user.ts | Switches to RegistryInterfaceRef. |
| apps/ensapi/src/omnigraph-api/schema/registration.ts | Updates wrapped token id resolution to load ENSv1 domain and use domain.node. |
| apps/ensapi/src/omnigraph-api/schema/query.ts | Retargets v1/v2 domain list queries to unified table; updates Query.registry to resolve concrete registries via DB lookup. |
| apps/ensapi/src/omnigraph-api/schema/query.integration.test.ts | Updates fixtures for ENSv1 id format (registry + node). |
| apps/ensapi/src/omnigraph-api/schema/domain.ts | Reworks Domain loader to unified domain table, adds Domain.parent, and updates ENSv1/ENSv2 type implementations. |
| apps/ensapi/src/omnigraph-api/lib/get-domain-by-interpreted-name.ts | Replaces separate v1/v2 lookup logic with unified forward traversal rooted at v1/v2 root registries. |
| apps/ensapi/src/omnigraph-api/lib/get-canonical-path.ts | Unifies canonical-path resolution into a single reverse-traversal CTE over domain + registryCanonicalDomain. |
| apps/ensapi/src/omnigraph-api/lib/find-domains/layers/filter-by-registry.ts | Updates docs/semantics for unified registry filtering. |
| apps/ensapi/src/omnigraph-api/lib/find-domains/layers/filter-by-parent.ts | Updates docs to match unified model (behavior unchanged). |
| apps/ensapi/src/omnigraph-api/lib/find-domains/layers/filter-by-name.ts | Replaces v1/v2 union traversal with a single traversal over unified domain table. |
| apps/ensapi/src/omnigraph-api/lib/find-domains/layers/filter-by-canonical.ts | Switches canonical filtering to require membership in canonical registries set. |
| apps/ensapi/src/omnigraph-api/lib/find-domains/layers/base-domain-set.ts | Replaces v1/v2 union base set with a single unified domain-based base set. |
| apps/ensapi/src/omnigraph-api/lib/find-domains/canonical-registries-cte.ts | Replaces ENSv2-only canonical registry traversal with unified traversal rooted at v1 root (+ v2 root if present). |
| apps/ensapi/src/omnigraph-api/context.ts | Consolidates canonical-path loaders into a single canonicalPath loader. |
| SPEC-domain-model.md | Adds/updates design spec for unified polymorphic domain + registry model and migration notes. |
| .changeset/unified-domain-model.md | Declares major bumps and documents breaking changes (schema + id formats + GraphQL). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@greptile re-review |
Greptile SummaryThis PR consolidates the separate
Confidence Score: 3/5Not safe to merge without addressing the canonical-filter regression and stale-column conflict update, both of which can silently produce wrong query results in production. Two P1 findings: filterByCanonical now depends on a complete subregistryId chain not guaranteed under out-of-order ENSv1 events; onConflictDoUpdate never refreshes registryId or node, leaving structural traversal columns potentially stale. apps/ensindexer/src/plugins/ensv2/handlers/ensv1/ENSv1Registry.ts and apps/ensapi/src/omnigraph-api/lib/find-domains/layers/filter-by-canonical.ts Important Files Changed
Entity Relationship Diagram%%{init: {'theme': 'neutral'}}%%
erDiagram
registry {
text id PK
RegistryType type
integer chainId
hex address
hex node "null for concrete registries"
}
domain {
text id PK
DomainType type
text registryId FK
text subregistryId FK
hex node "ENSv1 only"
bigint tokenId "ENSv2 only"
hex labelHash
hex ownerId
hex rootRegistryOwnerId "ENSv1 only"
}
registryCanonicalDomain {
text registryId PK
text domainId
}
registration {
text id PK
text domainId FK
}
registry ||--o{ domain : "registry (parent)"
registry ||--o{ domain : "subregistry (child)"
registryCanonicalDomain ||--|| registry : "points to"
registryCanonicalDomain ||--|| domain : "canonical domain for"
domain ||--o{ registration : "has"
|
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/enssdk/src/lib/types/ensv2.ts (1)
13-40: 🧹 Nitpick | 🔵 TrivialIntentional but fragile:
ENSv1VirtualRegistryIdandENSv1DomainIdare runtime-indistinguishable.Both are
${ENSv1RegistryId}/${node}strings differentiated only by TS brand. A given parent domain and its virtual registry produce byte-identical ids, so any future code path that deserializes these from the wire (e.g. GraphQL input, cache keys, cross-table joins) cannot reliably tell them apart without external context. The current comment documents the intent, but a runtime disambiguator (e.g. a distinct prefix likevreg:forENSv1VirtualRegistryId) would eliminate a class of future bugs.Not blocking — leaving as an optional consideration for the next iteration.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/enssdk/src/lib/types/ensv2.ts` around lines 13 - 40, The ENSv1VirtualRegistryId and ENSv1DomainId are currently byte-identical strings and should be made runtime-distinguishable: change the canonical serialized shape of ENSv1VirtualRegistryId to include a prefix (e.g. "vreg:") so it becomes `vreg:${ENSv1RegistryId}/${node}`, update any constructors/parsers/serializers/validators (places that build or parse ENSv1VirtualRegistryId) to add/strip that prefix, update comments and tests referring to ENSv1VirtualRegistryId, and ensure GraphQL inputs, cache key code and cross-table join logic use the new prefixed form (or accept both forms during a compatibility transition if needed); keep the TypeScript brand types ENSv1VirtualRegistryId and ENSv1DomainId but document the new runtime format.packages/ensdb-sdk/src/ensindexer-abstract/ensv2.schema.ts (1)
191-247: 🧹 Nitpick | 🔵 TrivialPolymorphic invariants are documented but unenforced at the DB layer.
The
INVARIANTcomments onregistry.node(non-null iffENSv1VirtualRegistry) and ondomain.tokenId/domain.node(non-null iffENSv2Domain/ENSv1Domainrespectively) rely on handler enforcement. While this approach is working, a single typed helper per table (e.g.,insertENSv1Domain/insertENSv2Domain) that forces the discriminator + non-null fields together would eliminate the ad-hoc validity checks at every call site and make regressions easier to catch at write time.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ensdb-sdk/src/ensindexer-abstract/ensv2.schema.ts` around lines 191 - 247, Polymorphic invariants (registry.node, domain.tokenId, domain.node) are only documented and not enforced; add typed insert helpers that validate and set discriminators to prevent invalid writes. Implement functions like insertENSv1Domain and insertENSv2Domain (and insertENSv1VirtualRegistry if needed) which accept strongly-typed payloads, set the domain.type/registry.type discriminator, require and validate the non-null fields (for ENSv1Domain require node; for ENSv2Domain require tokenId; for ENSv1VirtualRegistry require node on registry), and call the underlying table insert; replace ad-hoc writes to domain and registry with these helpers so all writes enforce the invariants at the API layer. Ensure the helpers are named and exported clearly (insertENSv1Domain, insertENSv2Domain, insertENSv1VirtualRegistry) and add minimal runtime checks that throw descriptive errors when invariants are violated.apps/ensindexer/src/plugins/ensv2/handlers/ensv1/BaseRegistrar.ts (1)
39-51: 🧹 Nitpick | 🔵 TrivialUpdate stale
v1Domainreferences in the file's doc comment.The comment still describes the pre-refactor model (
v1Domain must exist,v1Domain.owner is _conditionally_ materialized,we're materializing the v1Domain's effective owner), but the handler now writes into the unifiedensIndexerSchema.domain. This is inconsistent with the PR's terminology transition and the renamed table.♻️ Suggested wording
- * Because they all technically have this ability, this logic avoids the invariant that an associated - * v1Domain must exist and instead the v1Domain.owner is _conditionally_ materialized. + * Because they all technically have this ability, this logic avoids the invariant that an associated + * Domain must exist and instead the Domain's owner is _conditionally_ materialized. * - * Technically each BaseRegistrar Registration also has an associated owner that we could keep track - * of, but because we're materializing the v1Domain's effective owner, we need not explicitly track - * it. When a preminted name is actually registered, the indexing logic will see that the v1Domain - * exists and materialize its effective owner correctly. + * Technically each BaseRegistrar Registration also has an associated owner that we could keep track + * of, but because we're materializing the Domain's effective owner, we need not explicitly track + * it. When a preminted name is actually registered, the indexing logic will see that the Domain + * exists and materialize its effective owner correctly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ensindexer/src/plugins/ensv2/handlers/ensv1/BaseRegistrar.ts` around lines 39 - 51, The file-level doc comment for BaseRegistrar is stale: it repeatedly refers to v1Domain and materializing v1Domain.owner, but the handler now writes into the unified ensIndexerSchema.domain; update the comment to use current terminology (replace v1Domain references with the unified ensIndexerSchema.domain/domain record and describe conditional materialization of the domain owner there) and mention BaseRegistrar (and BaseRegistrar-derived registrars/controllers) only in terms of how they can create preminted names that result in entries in ensIndexerSchema.domain rather than a separate v1Domain table.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.changeset/unified-domain-model.md:
- Line 14: The changeset currently documents makeENSv1DomainId as taking
(AccountId, Node) but semantically ENSv1DomainId is
`${ENSv1RegistryId}/${node}`; update the documentation to state the first
parameter is ENSv1RegistryId (i.e., change the signature description to
`(ENSv1RegistryId, Node)`) while noting the implementation still accepts
AccountId-convertible values if you want, and keep the note that makeRegistryId
remains as a union-returning helper for callers that cannot narrow to specific
registry constructors like makeENSv1RegistryId, makeENSv2RegistryId, and
makeENSv1VirtualRegistryId.
In `@AGENTS.md`:
- Line 85: Update the line that instructs running `pnpm generate:gqlschema` to
specify when it's required: state that developers must run `pnpm
generate:gqlschema` whenever GraphQL SDL/schema files, GraphQL-related
TypeScript types, resolver signatures, queries/mutations/subscriptions, or any
files that define the GraphQL schema are changed (i.e., adding/removing fields,
types, directives, or changing type shapes), so that generated schema and types
stay in sync with source; reference the existing command `pnpm
generate:gqlschema` and replace "was affected" with this explicit list of change
types in AGENTS.md.
- Line 84: The sentence "If OpenAPI Specs were affected, run `pnpm
generate:openapi`" is ambiguous and inconsistent with existing CI wording;
change it to use lowercase "specs" and be more specific about when to run the
command (e.g., when you modified ENSApi routes or request/response types).
Update the line that currently reads "If OpenAPI Specs were affected, run `pnpm
generate:openapi`" to something like "If you modified ENSApi routes or
request/response types, run `pnpm generate:openapi`" and use "specs" rather than
"Specs" to match the CI messages.
In `@apps/ensapi/src/omnigraph-api/schema/domain.ts`:
- Around line 70-74: The type guards isENSv1Domain and isENSv2Domain declare
(domain: unknown) but directly access (domain as DomainInterface).type which can
throw on null/undefined or mis-handle primitives; update each guard to first
confirm domain is a non-null object and has a "type" property (e.g., typeof
domain === "object" && domain !== null && "type" in domain && typeof (domain as
any).type === "string") before comparing to "ENSv1Domain"/"ENSv2Domain", or
alternatively change the parameter type from unknown to DomainInterface if
callers never pass unknown.
In `@apps/ensindexer/src/lib/ensv2/domain-db-helpers.ts`:
- Around line 19-22: The inline comment above the update call is stale—update or
remove it so it no longer references `v1Domain`; change the comment to reflect
that you're updating the unified `ensIndexerSchema.domain` table (or delete the
comment). Locate the update invocation using
`context.ensDb.update(ensIndexerSchema.domain, { id }).set({ ownerId:
interpretAddress(owner) })` and replace the comment accordingly.
In `@apps/ensindexer/src/lib/managed-names.test.ts`:
- Around line 68-74: The test for getManagedName currently checks that the
BaseRegistrar instance (registrar) returns { name: "eth", node: ETH_NODE,
registry: ensv1Registry } but omits a parallel assertion for the
LegacyEthRegistrarController; add an assertion that calling
getManagedName(controller) (or the test variable representing
LegacyEthRegistrarController) returns an object whose registry field is the same
ensv1Registry (and optionally name/node equal to the expected values) so the
controller canonicalizes to the same registry as the registrar.
In `@SPEC-domain-model.md`:
- Line 437: Fix the typo in the documentation snippet for the RegistryIdInput
AccountId resolver: change the filter fragment `'ENSv1Registry',
'ENSv2Registry)` to include the missing closing quote so it reads
`'ENSv1Registry', 'ENSv2Registry'`; reference the same logical resolver name
"RegistryIdInput AccountId resolver" (the actual implementation uses
["ENSv1Registry","ENSv2Registry"]) to verify consistency and update the markdown
accordingly.
- Around line 36-42: Add blank lines before and after the fenced code blocks and
include a language hint for the dependency graph fence to satisfy markdownlint
(MD031/MD040). Specifically, in the snippet containing getManagedName(registry)
and makeENSv1VirtualRegistryId(registry, parentNode) ensure there is an empty
line above the opening ```ts and below the closing ```; apply the same
blank-line padding to the other fenced blocks around the dependency graph and
annotate that graph fence with a language tag such as ```text.
---
Outside diff comments:
In `@apps/ensindexer/src/plugins/ensv2/handlers/ensv1/BaseRegistrar.ts`:
- Around line 39-51: The file-level doc comment for BaseRegistrar is stale: it
repeatedly refers to v1Domain and materializing v1Domain.owner, but the handler
now writes into the unified ensIndexerSchema.domain; update the comment to use
current terminology (replace v1Domain references with the unified
ensIndexerSchema.domain/domain record and describe conditional materialization
of the domain owner there) and mention BaseRegistrar (and BaseRegistrar-derived
registrars/controllers) only in terms of how they can create preminted names
that result in entries in ensIndexerSchema.domain rather than a separate
v1Domain table.
In `@packages/ensdb-sdk/src/ensindexer-abstract/ensv2.schema.ts`:
- Around line 191-247: Polymorphic invariants (registry.node, domain.tokenId,
domain.node) are only documented and not enforced; add typed insert helpers that
validate and set discriminators to prevent invalid writes. Implement functions
like insertENSv1Domain and insertENSv2Domain (and insertENSv1VirtualRegistry if
needed) which accept strongly-typed payloads, set the domain.type/registry.type
discriminator, require and validate the non-null fields (for ENSv1Domain require
node; for ENSv2Domain require tokenId; for ENSv1VirtualRegistry require node on
registry), and call the underlying table insert; replace ad-hoc writes to domain
and registry with these helpers so all writes enforce the invariants at the API
layer. Ensure the helpers are named and exported clearly (insertENSv1Domain,
insertENSv2Domain, insertENSv1VirtualRegistry) and add minimal runtime checks
that throw descriptive errors when invariants are violated.
In `@packages/enssdk/src/lib/types/ensv2.ts`:
- Around line 13-40: The ENSv1VirtualRegistryId and ENSv1DomainId are currently
byte-identical strings and should be made runtime-distinguishable: change the
canonical serialized shape of ENSv1VirtualRegistryId to include a prefix (e.g.
"vreg:") so it becomes `vreg:${ENSv1RegistryId}/${node}`, update any
constructors/parsers/serializers/validators (places that build or parse
ENSv1VirtualRegistryId) to add/strip that prefix, update comments and tests
referring to ENSv1VirtualRegistryId, and ensure GraphQL inputs, cache key code
and cross-table join logic use the new prefixed form (or accept both forms
during a compatibility transition if needed); keep the TypeScript brand types
ENSv1VirtualRegistryId and ENSv1DomainId but document the new runtime format.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: caabc391-bd5b-42b3-b8cf-147088c9ee0d
⛔ Files ignored due to path filters (2)
packages/enssdk/src/omnigraph/generated/introspection.tsis excluded by!**/generated/**packages/enssdk/src/omnigraph/generated/schema.graphqlis excluded by!**/generated/**
📒 Files selected for processing (36)
.changeset/query-root-nonnull.md.changeset/unified-domain-model.mdAGENTS.mdSPEC-domain-model.mdapps/ensapi/src/omnigraph-api/context.tsapps/ensapi/src/omnigraph-api/lib/find-domains/canonical-registries-cte.tsapps/ensapi/src/omnigraph-api/lib/find-domains/layers/base-domain-set.tsapps/ensapi/src/omnigraph-api/lib/find-domains/layers/filter-by-canonical.tsapps/ensapi/src/omnigraph-api/lib/find-domains/layers/filter-by-name.tsapps/ensapi/src/omnigraph-api/lib/find-domains/layers/filter-by-parent.tsapps/ensapi/src/omnigraph-api/lib/find-domains/layers/filter-by-registry.tsapps/ensapi/src/omnigraph-api/lib/get-canonical-path.tsapps/ensapi/src/omnigraph-api/lib/get-domain-by-interpreted-name.tsapps/ensapi/src/omnigraph-api/schema/domain.integration.test.tsapps/ensapi/src/omnigraph-api/schema/domain.tsapps/ensapi/src/omnigraph-api/schema/query.integration.test.tsapps/ensapi/src/omnigraph-api/schema/query.tsapps/ensapi/src/omnigraph-api/schema/registration.tsapps/ensapi/src/omnigraph-api/schema/registry-permissions-user.tsapps/ensapi/src/omnigraph-api/schema/registry.tsapps/ensindexer/src/lib/ensv2/domain-db-helpers.tsapps/ensindexer/src/lib/managed-names.test.tsapps/ensindexer/src/lib/managed-names.tsapps/ensindexer/src/plugins/ensv2/handlers/ensv1/BaseRegistrar.tsapps/ensindexer/src/plugins/ensv2/handlers/ensv1/ENSv1Registry.tsapps/ensindexer/src/plugins/ensv2/handlers/ensv1/NameWrapper.tsapps/ensindexer/src/plugins/ensv2/handlers/ensv1/RegistrarController.tsapps/ensindexer/src/plugins/ensv2/handlers/ensv2/ENSv2Registry.tsapps/ensindexer/src/plugins/protocol-acceleration/handlers/ENSv1Registry.tsapps/ensindexer/src/plugins/protocol-acceleration/handlers/ThreeDNSToken.tspackages/ensdb-sdk/src/ensindexer-abstract/ensv2.schema.tspackages/ensdb-sdk/src/lib/drizzle.test.tspackages/ensnode-sdk/src/shared/root-registry.tspackages/ensnode-sdk/src/shared/types.tspackages/enssdk/src/lib/ids.tspackages/enssdk/src/lib/types/ensv2.ts
💤 Files with no reviewable changes (1)
- apps/ensapi/src/omnigraph-api/lib/find-domains/layers/filter-by-parent.ts
There was a problem hiding this comment.
Pull request overview
This PR unifies ENSv1/ENSv2 domain + registry modeling across the indexer DB schema, SDK IDs/types, and the Omnigraph GraphQL API to better support canonical traversal, polymorphic querying, and cross-registry conflict handling.
Changes:
- Merge
v1Domain/v2Domaininto a single polymorphicdomaintable and makeRegistrypolymorphic (v1 concrete, v1 virtual, v2). - Update ID types/constructors (notably CAIP-shaped
ENSv1DomainIdand new v1/v2 registry ID helpers). - Refactor ENSApi omnigraph schema/resolvers and find-domains traversal logic to operate over unified tables + a single canonical-path loader.
Reviewed changes
Copilot reviewed 36 out of 38 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/enssdk/src/omnigraph/generated/schema.graphql | Updates GraphQL schema (Domain.parent/path semantics, Registry interface + implementations, root non-null). |
| packages/enssdk/src/omnigraph/generated/introspection.ts | Regenerated introspection for new Domain/Registry polymorphism and field changes. |
| packages/enssdk/src/lib/types/ensv2.ts | Refactors RegistryId/DomainId-related branded types; introduces v1/v2 registry IDs + v1 virtual registry ID. |
| packages/enssdk/src/lib/ids.ts | Adds makeENSv1RegistryId/makeENSv2RegistryId/makeENSv1VirtualRegistryId; changes makeENSv1DomainId signature. |
| packages/ensnode-sdk/src/shared/types.ts | Adds RequiredAndNull helper type for discriminator-driven null invariants. |
| packages/ensnode-sdk/src/shared/root-registry.ts | Adds ENSv1 root registry ID helpers and getRootRegistryId() (prefer v2, fallback v1). |
| packages/ensdb-sdk/src/lib/drizzle.test.ts | Updates schema cloning tests for renamed/merged domain table. |
| packages/ensdb-sdk/src/ensindexer-abstract/ensv2.schema.ts | Implements unified domains table and polymorphic registries (type discriminator + v1 virtual node). |
| apps/ensindexer/src/plugins/protocol-acceleration/handlers/ThreeDNSToken.ts | Updates v1 domain ID construction to include concrete registry. |
| apps/ensindexer/src/plugins/protocol-acceleration/handlers/ENSv1Registry.ts | Canonicalizes v1 registry identity and uses CAIP-shaped v1 domain IDs. |
| apps/ensindexer/src/plugins/ensv2/handlers/ensv2/ENSv2Registry.ts | Writes unified domain rows for ENSv2 and inserts typed registry rows. |
| apps/ensindexer/src/plugins/ensv2/handlers/ensv1/RegistrarController.ts | Updates v1 domain ID construction to include concrete registry via managed-name canonicalization. |
| apps/ensindexer/src/plugins/ensv2/handlers/ensv1/NameWrapper.ts | Updates v1 domain ID construction to include concrete registry; removes duplicated managed-name lookup. |
| apps/ensindexer/src/plugins/ensv2/handlers/ensv1/ENSv1Registry.ts | Major rewrite: writes unified domain, manages v1 virtual registries, and maintains canonical-domain links for traversal. |
| apps/ensindexer/src/plugins/ensv2/handlers/ensv1/BaseRegistrar.ts | Retargets reads to unified domain table and updates v1 domain IDs to include concrete registry. |
| apps/ensindexer/src/lib/managed-names.ts | Reworks managed-name mapping to return {name,node,registry} and includes concrete registry contracts in groups. |
| apps/ensindexer/src/lib/managed-names.test.ts | Updates tests for new managed-name return shape and caching behavior. |
| apps/ensindexer/src/lib/ensv2/domain-db-helpers.ts | Retargets v1 effective-owner materialization updates to unified domain table. |
| apps/ensapi/src/omnigraph-api/schema/registry.ts | Introduces Registry loadable interface + ENSv1/ENSv1Virtual/ENSv2 implementations and shared fields. |
| apps/ensapi/src/omnigraph-api/schema/registry-permissions-user.ts | Updates registry field to reference the Registry interface. |
| apps/ensapi/src/omnigraph-api/schema/registration.ts | Adjusts wrapped tokenId resolver to work with new v1 domain ID format via Domain dataloader. |
| apps/ensapi/src/omnigraph-api/schema/query.ts | Updates root registry semantics (non-null) and registry lookup by (chainId,address,type); retargets v1/v2 domain queries to unified table. |
| apps/ensapi/src/omnigraph-api/schema/query.integration.test.ts | Updates integration tests for v1 ID shape, Registry polymorphism, and canonical filtering. |
| apps/ensapi/src/omnigraph-api/schema/domain.ts | Converts Domain to a loadable interface over unified domain table; adds Domain.parent; exposes v1 node. |
| apps/ensapi/src/omnigraph-api/schema/domain.integration.test.ts | Adds coverage for leaf→root canonical path semantics and alias collapsing behavior. |
| apps/ensapi/src/omnigraph-api/lib/get-domain-by-interpreted-name.ts | Replaces separate v1/v2 resolution logic with unified forward traversal rooted at v1/v2 roots. |
| apps/ensapi/src/omnigraph-api/lib/get-canonical-path.ts | Replaces v1/v2 canonical path functions with a unified reverse traversal over domain + registryCanonicalDomain. |
| apps/ensapi/src/omnigraph-api/lib/find-domains/layers/filter-by-registry.ts | Updates docs to reflect registry filtering across unified domain model. |
| apps/ensapi/src/omnigraph-api/lib/find-domains/layers/filter-by-parent.ts | Cleans up outdated comments now that parent derivation is unified. |
| apps/ensapi/src/omnigraph-api/lib/find-domains/layers/filter-by-name.ts | Refactors name-path traversal to operate on unified domain table and canonical-domain edges. |
| apps/ensapi/src/omnigraph-api/lib/find-domains/layers/filter-by-canonical.ts | Switches canonical filtering to join against canonical registries over unified registry IDs. |
| apps/ensapi/src/omnigraph-api/lib/find-domains/layers/base-domain-set.ts | Replaces unioned base set with a single base query over unified domain table. |
| apps/ensapi/src/omnigraph-api/lib/find-domains/canonical-registries-cte.ts | Rebuilds canonical-registries CTE to start from v1 root (and optionally v2 root) over unified domain links. |
| apps/ensapi/src/omnigraph-api/context.ts | Replaces v1/v2 canonical-path loaders with a single canonical-path DataLoader. |
| SPEC-domain-model.md | Adds detailed design/spec for unified polymorphic domain + registry model and migration strategy. |
| AGENTS.md | Updates contributor guidance to regenerate OpenAPI and GraphQL schema when affected. |
| .changeset/unified-domain-model.md | Adds breaking-change changeset describing unified schema and ID changes across packages/apps. |
| .changeset/query-root-nonnull.md | Adds changeset documenting Query.root becoming non-null and fallback semantics. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…creteRegistryId
- New `makeConcreteRegistryId(accountId)` helper in enssdk returning
`ENSv1RegistryId | ENSv2RegistryId` (never virtual — virtual ids carry a `/node`
suffix that AccountId alone cannot produce).
- `Query.registry(by: { contract })` uses `makeConcreteRegistryId` directly; the
downstream dataloader handles existence. Virtual registries remain intentionally
unaddressable by AccountId.
- `Query.v1Domains` + `Query.v2Domains` dev fields consolidated into a single
polymorphic `Query.allDomains` dev field.
- enskit `Query.registry` cache resolver uses `makeConcreteRegistryId` for tighter
typing.
- query integration test: ETH_NODE constant replaces namehashInterpretedName calls.
- ENSv2 schema: refactored column-invariant docs into uniform "If this is X, ...,
otherwise null" form; added `TokenId` brand on `domain.tokenId`; expanded
descriptions to mention ThreeDNS Registries. AGENTS.md gains testing guidance.
- Delete SPEC-domain-model.md (spec subsumed by implementation + changeset).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…includes ThreeDNS Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR refactors the ENS namegraph model to unify ENSv1 + ENSv2 Domains into a single polymorphic domain table and to make Registries polymorphic (ENSv1 concrete, ENSv1 virtual-per-parent, ENSv2), enabling consistent canonical traversal and reducing cross-registry conflicts.
Changes:
- Introduces unified
domain/registrydatamodel (type-discriminated) and updates ENSv1 IDs to be CAIP-shaped (${ENSv1RegistryId}/${node}), plus new registry ID constructors (incl. virtual registries). - Updates Omnigraph GraphQL schema to reflect polymorphic Domain/Registry interfaces (adds
Domain.parent, exposesENSv1Domain.node, makesQuery.rootnon-null). - Reworks canonical-path and find-domains traversal to operate over the unified namegraph, and updates indexer handlers + tests accordingly.
Reviewed changes
Copilot reviewed 36 out of 38 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/enssdk/src/omnigraph/generated/schema.graphql | Updates public Omnigraph schema for polymorphic Domain/Registry and new fields/queries. |
| packages/enssdk/src/omnigraph/generated/introspection.ts | Regenerates introspection JSON for updated schema types/interfaces. |
| packages/enssdk/src/lib/types/ensv2.ts | Introduces branded registry ID variants; changes ENSv1DomainId to ${registryId}/${node} shape. |
| packages/enssdk/src/lib/ids.ts | Adds constructors for ENSv1/ENSv2/virtual registry IDs; updates makeENSv1DomainId signature. |
| packages/ensnode-sdk/src/shared/types.ts | Adds RequiredAndNull<T, K> helper type for polymorphic models. |
| packages/ensnode-sdk/src/shared/root-registry.ts | Adds v1 root helpers + getRootRegistryId() (prefer v2, fall back to v1). |
| packages/enskit/src/react/omnigraph/_lib/by-id-lookup-resolvers.ts | Updates client-side graphcache resolver to use concrete registry IDs (but still returns interface typename). |
| packages/ensdb-sdk/src/lib/drizzle.test.ts | Updates DB schema cloning tests to use unified domain table name. |
| packages/ensdb-sdk/src/ensindexer-abstract/ensv2.schema.ts | Unifies v1/v2 domain tables into domains; adds registry polymorphism + virtual registry support. |
| apps/ensindexer/src/plugins/protocol-acceleration/handlers/ThreeDNSToken.ts | Updates ENSv1 domain ID creation to include concrete registry. |
| apps/ensindexer/src/plugins/protocol-acceleration/handlers/ENSv1Registry.ts | Canonicalizes ENSv1 registry selection via managed-name mapping when writing relationships. |
| apps/ensindexer/src/plugins/ensv2/handlers/ensv2/ENSv2Registry.ts | Writes ENSv2 domains into unified table; inserts typed ENSv2 registry rows. |
| apps/ensindexer/src/plugins/ensv2/handlers/ensv1/RegistrarController.ts | Updates ENSv1 domain ID creation to include concrete registry from managed-name mapping. |
| apps/ensindexer/src/plugins/ensv2/handlers/ensv1/NameWrapper.ts | Updates ENSv1 domain ID creation to include concrete registry; aligns wrapper flows with unified IDs. |
| apps/ensindexer/src/plugins/ensv2/handlers/ensv1/ENSv1Registry.ts | Implements ENSv1 virtual registries + writes ENSv1 domains into unified table with typed registries. |
| apps/ensindexer/src/plugins/ensv2/handlers/ensv1/BaseRegistrar.ts | Updates ENSv1 domain ID creation and DB lookups to unified domain table. |
| apps/ensindexer/src/lib/managed-names.ts | Refactors managed-name groups to include canonical concrete ENSv1 registry per group. |
| apps/ensindexer/src/lib/managed-names.test.ts | Updates tests for managed-name mapping to include registry in results. |
| apps/ensindexer/src/lib/ensv2/domain-db-helpers.ts | Updates ENSv1 owner materialization helper to write to unified domain table. |
| apps/ensapi/src/omnigraph-api/schema/registry.ts | Converts Registry to a loadable interface with concrete implementations (v1/v1-virtual/v2). |
| apps/ensapi/src/omnigraph-api/schema/registry-permissions-user.ts | Updates registry field type to the new Registry interface ref. |
| apps/ensapi/src/omnigraph-api/schema/registration.ts | Updates wrapped tokenId derivation by loading ENSv1 domain and using its node. |
| apps/ensapi/src/omnigraph-api/schema/query.ts | Adds allDomains dev query; makes root non-null and returns preferred root via getRootRegistryId. |
| apps/ensapi/src/omnigraph-api/schema/query.integration.test.ts | Updates integration tests for new root behavior, registry polymorphism, canonical filtering, and ENSv1 node. |
| apps/ensapi/src/omnigraph-api/schema/domain.ts | Converts Domain to a loadable interface over unified table; adds parent and canonical-path loader usage. |
| apps/ensapi/src/omnigraph-api/schema/domain.integration.test.ts | Adds integration tests validating canonical path (leaf→root) and alias collapsing behavior. |
| apps/ensapi/src/omnigraph-api/lib/get-domain-by-interpreted-name.ts | Unifies forward traversal (v1/v2) via a single recursive CTE using domain.subregistryId. |
| apps/ensapi/src/omnigraph-api/lib/get-canonical-path.ts | Unifies canonical path derivation via reverse traversal over unified domain + root registry termination. |
| apps/ensapi/src/omnigraph-api/lib/find-domains/layers/filter-by-registry.ts | Updates docs/logic to reflect unified domains-by-registry filtering. |
| apps/ensapi/src/omnigraph-api/lib/find-domains/layers/filter-by-parent.ts | Updates docs/logic for unified parent derivation via canonical traversal. |
| apps/ensapi/src/omnigraph-api/lib/find-domains/layers/filter-by-name.ts | Unifies name-path filtering (v1/v2) into a single canonical traversal CTE over domain. |
| apps/ensapi/src/omnigraph-api/lib/find-domains/layers/filter-by-canonical.ts | Switches canonical filtering to rely on canonical registries CTE via inner join. |
| apps/ensapi/src/omnigraph-api/lib/find-domains/layers/base-domain-set.ts | Replaces v1/v2 union base set with a unified domain-based base set + authenticated parent derivation. |
| apps/ensapi/src/omnigraph-api/lib/find-domains/canonical-registries-cte.ts | Expands canonical registries CTE to include v1 root subtree and (optionally) v2 root subtree. |
| apps/ensapi/src/omnigraph-api/context.ts | Replaces separate v1/v2 canonical-path loaders with a single canonicalPath loader. |
| AGENTS.md | Adds guidance for running generators and preferred test assertion style. |
| .changeset/unified-domain-model.md | Documents breaking ID/schema changes and GraphQL interface changes for the unified model. |
| .changeset/query-root-nonnull.md | Documents Query.root becoming non-null with v2-preferred/v1-fallback behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
…raphcache fixes - New `getRootRegistryIds(namespace)` helper returns every top-level Root Registry (all concrete ENSv1Registries + ENSv2 Root when defined). ENSv1 is multi-rooted on disk: Basenames/Lineanames shadow Registries root their own subtrees and are not linked to the mainnet subtree at the indexed-namegraph level. - `getDomainIdByInterpretedName` now forward-traverses from every Root in parallel, preferring the v2 hit and otherwise any v1 hit. Fixes a bug where shadow-registry names (e.g. foo.base.eth) were unreachable via `Query.domain(by: name)`. - `canonical-registries-cte` extends the seed union to every top-level Root. - `isENSv1Domain`/`isENSv2Domain` tightened to `(domain: DomainInterface)` — pushes null-handling to callers; Pothos `isTypeOf` casts at the boundary. - `WrappedBaseRegistrarRegistration.tokenId` explicit null-check before the ENSv1Domain invariant, so a missing domain raises the right error rather than TypeError-ing on `domain.type` interpolation. - enskit `Query.registry` cache resolver probes concrete `__typename`s (ENSv1Registry / ENSv2Registry / ENSv1VirtualRegistry) — `Registry` is a GraphQL interface; graphcache normalizes on the concrete type. - Typo + stale-comment cleanup (Manage Name → Managed Name; v1Domain's → Domain's; drop stale expiry-TODO that contradicts the file's docstring). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@greptile re-review |
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
apps/ensindexer/src/plugins/ensv2/handlers/ensv1/ENSv1Registry.ts (1)
103-113:⚠️ Potential issue | 🟡 MinorParent-domain
subregistryIdupdate silently no-ops if the parent row is missing.Line 104-106 uses
.update(ensIndexerSchema.domain, { id: parentDomainId }).set(...)with no guard that the parent row exists. IfparentDomainId's row hasn't been written yet, the update silently succeeds with 0 rows — the parent is then "canonical-linked" viaregistryCanonicalDomainon line 109-112 but has nosubregistryIdpointer, breaking forward descent viadomain.subregistryId → registryCanonicalDomain → subdomain.Under strict in-order indexing this cannot happen: the parent's own
NewOwnerevent runs first and inserts the parent intoensIndexerSchema.domain. But consider making this assumption explicit — e.g. anensureDomain-style insert-or-no-op before the update, or an invariant throw if the update affected zero rows — so that any future ordering change (different managed-name bootstrapping, reorg/backfill, ENSv1RegistryOld→ENSv1Registry migration edge cases) fails loudly instead of silently producing an unreachable subtree.As per coding guidelines: "Fail fast and loudly on invalid inputs for validation."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ensindexer/src/plugins/ensv2/handlers/ensv1/ENSv1Registry.ts` around lines 103 - 113, The update to set domain.subregistryId using context.ensDb.update(ensIndexerSchema.domain, { id: parentDomainId }).set({ subregistryId: parentRegistryId }) can silently affect 0 rows if the parent domain row doesn't exist; change the logic to guarantee the parent exists or fail loudly: before the update either perform an insert-or-no-op (e.g. ensureDomain-style upsert into ensIndexerSchema.domain for parentDomainId) or execute the update and check the result row count and throw an error if zero, and only then insert/update ensIndexerSchema.registryCanonicalDomain; this will make functions like ENSv1Registry (parentDomainId, parentRegistryId) fail fast instead of leaving a canonical link without a subregistryId.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@AGENTS.md`:
- Around line 87-90: The file contains a duplicate "## Testing" heading; merge
the two by moving the two bullets ("Prefer the `await expect(...).resolves.*`
format over await-then-expect." and "Prefer `await
expect(...).resolves.toMatchObject({})` over expecting individual properties, if
it is more concise.") into the existing "## Testing" section (the original "##
Testing" header) and delete the duplicate "## Testing" section at the bottom so
there is a single consolidated testing section.
In `@apps/ensapi/src/omnigraph-api/lib/find-domains/canonical-registries-cte.ts`:
- Around line 42-44: The reduce over roots to build rootsUnion can throw when
roots is empty; update getRootRegistryIds to guard and fail fast by checking
roots.length > 0 and throwing a clear error (e.g. "no root registries
configured") before the reduce, or provide a safe initial value to the reduce so
it never calls reduce on an empty array; locate the roots variable and the
rootsUnion construction in canonical-registries-cte.ts (the rootsUnion
definition) and implement one of these two fixes so an empty/misconfigured
namespace produces an explicit, descriptive failure instead of a TypeError.
- Around line 56-68: Update the JSDoc to reflect that the recursive CTE walks
Domain.subregistryId (i.e., "each Registry is reached via any Domain's
subregistryId within a canonical Registry, covering both ENSv1 virtual
registries and ENSv2 subregistries") instead of mentioning
registryCanonicalDomain, and modify the final CTE projection in
canonical_registries (the SELECT registry_id FROM canonical_registries) to
SELECT DISTINCT registry_id FROM canonical_registries to prevent duplicate
registry IDs from duplicating rows downstream (affects filterByCanonical's
innerJoin and pagination/counts); the change touches the canonical_registries
CTE (rootsUnion, CANONICAL_REGISTRIES_MAX_DEPTH) and relates to
filterByCanonical behavior.
In `@apps/ensapi/src/omnigraph-api/schema/query.integration.test.ts`:
- Around line 148-164: The test "filters by canonical" in
query.integration.test.ts is too weak—add a negative assertion to ensure
non-canonical domains are excluded: update the DEVNET_NAMES test fixtures to
include a known non-canonical domain (one that should not be canonical under the
logic in canonical-registries-cte.ts), then in the "filters by canonical" test
(the QueryDomains invocation and flattenConnection usage) assert that this
non-canonical fixture is not present in the returned domains (e.g., find by
name/id and expect it toBeUndefined or not found) so that leakage of
non-canonical v2 entries is detected.
In `@apps/ensapi/src/omnigraph-api/schema/registry.ts`:
- Around line 48-55: Change the three type-guard signatures isENSv1Registry,
isENSv1VirtualRegistry, and isENSv2Registry from (registry: unknown) to
(registry: RegistryInterface) so they no longer rely on unsafe casts and mirror
the domain.ts guards; update the callers that pass values into these guards (the
places using them as isTypeOf) to cast their argument to RegistryInterface
(e.g., value as RegistryInterface) before calling the guard, keeping the guard
implementations unchanged except for the parameter type.
In `@packages/ensnode-sdk/src/shared/root-registry.ts`:
- Around line 128-139: getRootRegistryIds currently omits ThreeDNS, so
ThreeDNSToken contract addresses must be included as top-level v1 registries;
update getRootRegistryIds to add maybeGetDatasourceContract(namespace,
DatasourceNames.ThreeDNSBase, "ThreeDNSToken") and
maybeGetDatasourceContract(namespace, DatasourceNames.ThreeDNSOptimism,
"ThreeDNSToken") into the v1Registries array (same filter/map to AccountId and
makeENSv1RegistryId) so ThreeDNS domain subtrees are reachable by
canonical-registries-cte/getDomainIdByInterpretedName; alternatively, if you
prefer the other approach, narrow the schema comment in ensv2.schema.ts to
remove ThreeDNS from the list of concrete ENSv1Registry rows.
---
Duplicate comments:
In `@apps/ensindexer/src/plugins/ensv2/handlers/ensv1/ENSv1Registry.ts`:
- Around line 103-113: The update to set domain.subregistryId using
context.ensDb.update(ensIndexerSchema.domain, { id: parentDomainId }).set({
subregistryId: parentRegistryId }) can silently affect 0 rows if the parent
domain row doesn't exist; change the logic to guarantee the parent exists or
fail loudly: before the update either perform an insert-or-no-op (e.g.
ensureDomain-style upsert into ensIndexerSchema.domain for parentDomainId) or
execute the update and check the result row count and throw an error if zero,
and only then insert/update ensIndexerSchema.registryCanonicalDomain; this will
make functions like ENSv1Registry (parentDomainId, parentRegistryId) fail fast
instead of leaving a canonical link without a subregistryId.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6abde8fa-b5c6-4859-9061-c3d7f72c38fa
⛔ Files ignored due to path filters (2)
packages/enssdk/src/omnigraph/generated/introspection.tsis excluded by!**/generated/**packages/enssdk/src/omnigraph/generated/schema.graphqlis excluded by!**/generated/**
📒 Files selected for processing (14)
AGENTS.mdapps/ensapi/src/omnigraph-api/lib/find-domains/canonical-registries-cte.tsapps/ensapi/src/omnigraph-api/lib/get-domain-by-interpreted-name.tsapps/ensapi/src/omnigraph-api/schema/domain.tsapps/ensapi/src/omnigraph-api/schema/query.integration.test.tsapps/ensapi/src/omnigraph-api/schema/query.tsapps/ensapi/src/omnigraph-api/schema/registration.tsapps/ensapi/src/omnigraph-api/schema/registry.tsapps/ensindexer/src/lib/ensv2/domain-db-helpers.tsapps/ensindexer/src/plugins/ensv2/handlers/ensv1/ENSv1Registry.tspackages/ensdb-sdk/src/ensindexer-abstract/ensv2.schema.tspackages/enskit/src/react/omnigraph/_lib/by-id-lookup-resolvers.tspackages/ensnode-sdk/src/shared/root-registry.tspackages/enssdk/src/lib/ids.ts
…s + nits - `getCanonicalPath` now terminates at any Root Registry returned by `getRootRegistryIds` — fixes the shadow-registry bug where `Domain.name`/`path`/`parent` all returned null for direct children of Basenames/Lineanames (e.g. foo.base.eth). Parallel to the fix already applied to forward traversal. - `canonical-registries-cte` outer projection uses `SELECT DISTINCT`. The reachable registry set is a DAG (aliased subregistries let multiple parent Domains declare the same child Registry), so the CTE can emit the same registry_id at multiple depths; without DISTINCT, `filterByCanonical`'s innerJoin multiplies base rows and inflates `$count`/pagination. Docstring rewritten to match reality (forward `subregistryId` walk, not `rcd`). - `isENSv1Registry`/`isENSv1VirtualRegistry`/`isENSv2Registry` signatures tightened to `RegistryInterface`; Pothos `isTypeOf` casts at the boundary (parallel to the Domain type-guard fix). - AGENTS.md Testing sections merged (dedup heading). - `it.todo` for negative canonical-filter assertion (devnet needs a known non-canonical fixture). - Drop ThreeDNS phrases from the ENSv1Registry descriptions (deferred to a future 3DNS integration PR). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR refactors ENSNode’s domain/registry model into a unified polymorphic representation across ENSv1 + ENSv2, updating DB schema, ID formats, and Omnigraph GraphQL types/resolvers to better handle cross-registry conflicts and canonical traversal.
Changes:
- Unifies
v1Domain+v2Domaininto a singledomaintable (discriminated bytype) and makesRegistrypolymorphic (ENSv1 concrete, ENSv1 virtual, ENSv2). - Updates ID formats and constructors (notably
ENSv1DomainId→${ENSv1RegistryId}/${node}and new registry ID helpers). - Reworks canonical traversal / domain lookup and updates Omnigraph GraphQL schema + generated artifacts accordingly.
Reviewed changes
Copilot reviewed 36 out of 38 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/enssdk/src/omnigraph/generated/schema.graphql | Updates Omnigraph SDL: Registry becomes an interface; Domain.parent added; Query.allDomains added; Query.root becomes non-null. |
| packages/enssdk/src/omnigraph/generated/introspection.ts | Regenerates introspection JSON to reflect new polymorphic Registry and unified Domain surface. |
| packages/enssdk/src/lib/types/ensv2.ts | Introduces branded ENSv1RegistryId/ENSv2RegistryId/ENSv1VirtualRegistryId and redefines RegistryId union; changes ENSv1DomainId to CAIP-shaped string. |
| packages/enssdk/src/lib/ids.ts | Adds new ID constructors (makeENSv1RegistryId, makeENSv2RegistryId, makeENSv1VirtualRegistryId, makeConcreteRegistryId); updates makeENSv1DomainId signature. |
| packages/ensnode-sdk/src/shared/types.ts | Adds RequiredAndNull<T, K> utility type. |
| packages/ensnode-sdk/src/shared/root-registry.ts | Adds helpers for ENSv1 root IDs and introduces getRootRegistryId + getRootRegistryIds to enumerate canonical roots. |
| packages/enskit/src/react/omnigraph/_lib/by-id-lookup-resolvers.ts | Updates graphcache resolvers to handle Registry as an interface and resolve concrete typenames via cache probing. |
| packages/ensdb-sdk/src/lib/drizzle.test.ts | Updates tests for renamed/merged schema table (v1Domain → domain). |
| packages/ensdb-sdk/src/ensindexer-abstract/ensv2.schema.ts | Implements unified domain + polymorphic registry tables; updates relations and indexes accordingly. |
| apps/ensindexer/src/plugins/protocol-acceleration/handlers/ThreeDNSToken.ts | Updates ENSv1 domain ID construction to include concrete registry. |
| apps/ensindexer/src/plugins/protocol-acceleration/handlers/ENSv1Registry.ts | Canonicalizes ENSv1 registry (old vs new) when computing ENSv1 domain IDs for resolver relations. |
| apps/ensindexer/src/plugins/ensv2/handlers/ensv2/ENSv2Registry.ts | Writes registry.type, migrates inserts/updates/finds from v2Domain to unified domain, and uses ENSv2-specific registry IDs. |
| apps/ensindexer/src/plugins/ensv2/handlers/ensv1/RegistrarController.ts | Updates ENSv1 domain ID construction to include concrete registry from managed-name group. |
| apps/ensindexer/src/plugins/ensv2/handlers/ensv1/NameWrapper.ts | Updates ENSv1 domain ID construction (including wrapped tokenId→node case) to include concrete registry. |
| apps/ensindexer/src/plugins/ensv2/handlers/ensv1/ENSv1Registry.ts | Refactors ENSv1 NewOwner/NewResolver/NewTTL flows to unified domain + virtual registry modeling and canonical path metadata. |
| apps/ensindexer/src/plugins/ensv2/handlers/ensv1/BaseRegistrar.ts | Migrates lookups from v1Domain to unified domain and updates ENSv1 domain ID construction. |
| apps/ensindexer/src/lib/managed-names.ts | Refactors managed-name mapping to include concrete registry ownership metadata (and includes registry contracts in groups). |
| apps/ensindexer/src/lib/managed-names.test.ts | Updates tests for new registry return value and uses match-object assertions where appropriate. |
| apps/ensindexer/src/lib/ensv2/domain-db-helpers.ts | Updates ENSv1 effective owner materialization to write unified domain table. |
| apps/ensapi/src/omnigraph-api/schema/registry.ts | Converts Registry from object to loadable interface with ENSv1/ENSv1Virtual/ENSv2 implementations and shared fields. |
| apps/ensapi/src/omnigraph-api/schema/registry-permissions-user.ts | Updates registry field type to the new Registry interface. |
| apps/ensapi/src/omnigraph-api/schema/registration.ts | Fixes wrapped tokenId derivation by loading the ENSv1 domain and reading domain.node (since ENSv1DomainId is no longer the node). |
| apps/ensapi/src/omnigraph-api/schema/query.ts | Replaces v1Domains/v2Domains dev methods with allDomains; makes root non-null and updates registry polymorphism behavior. |
| apps/ensapi/src/omnigraph-api/schema/query.integration.test.ts | Updates tests for Query.root preference + registry polymorphism and ENSv1 domain IDs. |
| apps/ensapi/src/omnigraph-api/schema/domain.ts | Refactors to a unified loadable Domain interface backed by domain table; adds parent field via canonical path loader. |
| apps/ensapi/src/omnigraph-api/schema/domain.integration.test.ts | Adds/updates integration tests for canonical Domain.path semantics (leaf→root and alias collapsing). |
| apps/ensapi/src/omnigraph-api/lib/get-domain-by-interpreted-name.ts | Rewrites forward traversal to operate over unified domain table and traverse from all configured root registries. |
| apps/ensapi/src/omnigraph-api/lib/get-canonical-path.ts | Collapses v1/v2 canonical path logic into a single reverse traversal over unified domain + registryCanonicalDomain. |
| apps/ensapi/src/omnigraph-api/lib/find-domains/layers/filter-by-registry.ts | Updates docs/intent to reflect registry filtering over unified domain set. |
| apps/ensapi/src/omnigraph-api/lib/find-domains/layers/filter-by-parent.ts | Cleans up docs now that parent derivation is unified. |
| apps/ensapi/src/omnigraph-api/lib/find-domains/layers/filter-by-name.ts | Replaces dual v1/v2 traversal queries with a unified reverse-walk via registryCanonicalDomain. |
| apps/ensapi/src/omnigraph-api/lib/find-domains/layers/filter-by-canonical.ts | Simplifies canonical filtering to an inner-join against the canonical registries CTE (no more v1/v2 split). |
| apps/ensapi/src/omnigraph-api/lib/find-domains/layers/base-domain-set.ts | Replaces unioned v1/v2 base sets with a single base set built from domain and canonical-parent derivation. |
| apps/ensapi/src/omnigraph-api/lib/find-domains/canonical-registries-cte.ts | Rebuilds canonical registries CTE to traverse forward from all configured roots via domain.subregistryId. |
| apps/ensapi/src/omnigraph-api/context.ts | Replaces separate v1/v2 canonical-path dataloaders with a unified canonicalPath loader. |
| AGENTS.md | Adds test assertion style guidance and explicit “run generators” steps for OpenAPI/GraphQL schema changes. |
| .changeset/unified-domain-model.md | Documents breaking changes in IDs and the unified schema/interface redesign. |
| .changeset/query-root-nonnull.md | Documents Query.root becoming non-null and selecting preferred root registry. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ...(basenamesRegistry && { | ||
| "base.eth": { | ||
| registry: basenamesRegistry, | ||
| contracts: [ | ||
| basenamesRegistry, | ||
| maybeGetDatasourceContract(config.namespace, DatasourceNames.Basenames, "BaseRegistrar"), | ||
| maybeGetDatasourceContract( | ||
| config.namespace, | ||
| DatasourceNames.Basenames, | ||
| "EARegistrarController", | ||
| ), | ||
| maybeGetDatasourceContract( | ||
| config.namespace, | ||
| DatasourceNames.Basenames, | ||
| "RegistrarController", | ||
| ), | ||
| maybeGetDatasourceContract( | ||
| config.namespace, | ||
| DatasourceNames.Basenames, | ||
| "UpgradeableRegistrarController", | ||
| ), | ||
| ].filter((c) => !!c), | ||
| } satisfies ManagedNameGroup, |
| ...(lineanamesRegistry && { | ||
| "linea.eth": { | ||
| registry: lineanamesRegistry, | ||
| contracts: [ | ||
| lineanamesRegistry, | ||
| maybeGetDatasourceContract(config.namespace, DatasourceNames.Lineanames, "BaseRegistrar"), | ||
| maybeGetDatasourceContract( | ||
| config.namespace, | ||
| DatasourceNames.Lineanames, | ||
| "EthRegistrarController", | ||
| ), | ||
| lineanamesNameWrapper, | ||
| ].filter((c) => !!c), | ||
| } satisfies ManagedNameGroup, | ||
| }), |
| WITH RECURSIVE canonical_registries AS ( | ||
| SELECT ${getENSV2RootRegistryId()}::text AS registry_id, 0 AS depth | ||
| ${rootsUnion} | ||
| UNION ALL | ||
| SELECT rcd.registry_id, cr.depth + 1 | ||
| FROM ${ensIndexerSchema.registryCanonicalDomain} rcd | ||
| JOIN ${ensIndexerSchema.v2Domain} parent ON parent.id = rcd.domain_id AND parent.subregistry_id = rcd.registry_id | ||
| JOIN canonical_registries cr ON cr.registry_id = parent.registry_id | ||
| -- Filter nulls at the recursive step so terminal Domains (no subregistry declared) don't | ||
| -- emit null rows into the CTE and don't spawn dead-end recursion branches. | ||
| SELECT d.subregistry_id AS registry_id, cr.depth + 1 | ||
| FROM canonical_registries cr | ||
| JOIN ${ensIndexerSchema.domain} d ON d.registry_id = cr.registry_id | ||
| WHERE cr.depth < ${CANONICAL_REGISTRIES_MAX_DEPTH} | ||
| AND d.subregistry_id IS NOT NULL | ||
| ) |
| // Canonicalize ENSv1Registry vs. ENSv1RegistryOld via `getManagedName(...).registry`. Both | ||
| // Registries share a Managed Name (the ENS Root for mainnet) and write into the same | ||
| // namegraph; canonicalizing here ensures Old events that pass `nodeIsMigrated` don't fragment | ||
| // domains across two Registry IDs. | ||
| const { node: managedNode, registry } = getManagedName(getThisAccountId(context, event)); |
| return ensDb | ||
| .select(selectBase(base)) | ||
| .from(base) | ||
| .leftJoin(canonicalRegistries, eq(canonicalRegistries.id, base.registryId)) | ||
| .where( | ||
| or( | ||
| isNull(base.registryId), // v1 domains are always canonical | ||
| isNotNull(canonicalRegistries.id), // v2 domains must be in a canonical registry | ||
| ), | ||
| ) | ||
| .innerJoin(canonicalRegistries, eq(canonicalRegistries.id, base.registryId)) | ||
| .as("baseDomains"); | ||
| } |
There was a problem hiding this comment.
ENSv1 domains with unset
subregistryId silently fall out of canonical results
filterByCanonical switched from a LEFT JOIN … WHERE (registryId IS NULL OR canonical) pattern to a plain INNER JOIN. In the new model every domain — including ENSv1 — has a non-null registryId, so ENSv1 domains will only appear in the canonical set when their registryId is reachable from a root via canonical_registries. That reachability depends on domain.subregistryId being correctly set on every ancestor. If a child NewOwner fires before its parent exists, the UPDATE ... SET subregistryId is a no-op, and any descendant of that gap will have a registryId never emitted by the CTE, disappearing from canonical results. The old registryId IS NULL pass-through was a safe default; the new strict inner-join makes results sensitive to indexing event order.
closes #205
closes #1511
closes #1877